Skip to content

chore(nextjs): Fix server actions detection - #3995

Closed
panteliselef wants to merge 3 commits into
mainfrom
elef/fix-server-action-detection
Closed

chore(nextjs): Fix server actions detection#3995
panteliselef wants to merge 3 commits into
mainfrom
elef/fix-server-action-detection

Conversation

@panteliselef

@panteliselef panteliselef commented Aug 21, 2024

Copy link
Copy Markdown
Contributor

Description

isServerActionRequest was still depending on the next-url header which no longer is available (removed at 14.0.4 for server actions and in 14.2.3 when requesting RSCs (Related PR)

This PR fixes the issue and prepares the ground for future work that would require these utilities to work properly.

pagePath from __nextGetStaticStore is available since next@13.5.4 until next@14.2.5 which is the latest stable release

Checklist

  • npm test runs as expected.
  • npm run build runs as expected.
  • (If applicable) JSDoc comments have been added or updated for any package exports
  • (If applicable) Documentation has been updated

Type of change

  • 🐛 Bug fix
  • 🌟 New feature
  • 🔨 Breaking change
  • 📖 Refactoring / dependency upgrade / documentation
  • other:

`isServerActionRequest` was still depending on the `next-url` header which no longer is available (removed at `14.1.0` for server actions and in `14.2.2` when requesting RSCs.

This PR fixes the issue and prepares the ground for future work that would require these utilities to work properly.

`pagePath` from __nextGetStaticStore is available since `next@13.5.4` until `next@14.2.5` which is the latest stable release
@panteliselef panteliselef self-assigned this Aug 21, 2024
@changeset-bot

changeset-bot Bot commented Aug 21, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 128e3c8

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@clerk/nextjs Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

return (
!!req.headers.get(nextConstants.Headers.NextUrl) &&
(req.headers.get(constants.Headers.Accept)?.includes('text/x-component') ||
req.headers.get(constants.Headers.ContentType)?.includes('multipart/form-data') ||

@panteliselef panteliselef Aug 21, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking against multipart is not realistic. The following example will fire a request with Content-type text/plain;charset=UTF-8

'use client';

import { addItem2 } from '../actions';

export default function AddToCart() {
  return (
    <button
      type='button'
      onClick={() => {
        addItem2({ some: 'data' });
      }}
    >
      Click
    </button>
  );
}


const isServerActionRequest = (req: Request) => {
return (
!!req.headers.get(nextConstants.Headers.NextUrl) &&

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next-url as a header has been dropped from request to server actions since 14.1.0 way before 14.2.2 where our page detection started to fail.

* github.com/vercel/next.js/blob/0ac10d79720cc950df96bd9d4958c9be0c075b6f/packages/next/src/lib/is-app-route-route.ts
* In case we want to handle router handlers and server actions differently in the future
*/
export function isAppRouteRoute(route: string): boolean {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I think they are called "route handlers": https://nextjs.org/docs/app/building-your-application/routing/route-handlers

Suggested change
export function isAppRouteRoute(route: string): boolean {
export function isAppRouteHandler(route: string): boolean {

Comment on lines +126 to +127
* *Attention*:
* When used within the Edge Middleware this utility will mistakenly detect a Route Handler as a Page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this problematic for our middleware in any scenarios you can think of?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panteliselef related to Bryce's question above, what does this comment mean for the middleware's auth() helper?

"@clerk/nextjs": patch
---

Fix server actions detection

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please state what's the impact for the end user here?

Comment on lines +126 to +127
* *Attention*:
* When used within the Edge Middleware this utility will mistakenly detect a Route Handler as a Page

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@panteliselef related to Bryce's question above, what does this comment mean for the middleware's auth() helper?

@panteliselef

Copy link
Copy Markdown
Contributor Author

Since we did not have any issue reports and seems like we will not be needing this in the near future, I will be closing this. We can always come back and reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants